bundle/dstate: nullify local state when DMS recording enabled - #6541
Draft
shreyas-goenka wants to merge 46 commits into
Draft
bundle/dstate: nullify local state when DMS recording enabled#6541shreyas-goenka wants to merge 46 commits into
shreyas-goenka wants to merge 46 commits into
Conversation
Record each deploy's resource state to the deployment metadata service and read it back, making the service the source of truth for a recorded bundle: the local state file becomes a tombstone and a leftover WAL is discarded. Gated on experimental.record_deployment_history (direct engine only), and supports deploy --plan. Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
…anged Co-authored-by: Isaac <no-reply@databricks.com>
…source Co-authored-by: Isaac <no-reply@databricks.com>
…, drop dedicated summary test Co-authored-by: Isaac <no-reply@databricks.com>
…op redundant operation-upload-message test Co-authored-by: Isaac <no-reply@databricks.com>
…, drop serialized-plan
…rces.get_recorded_state
… stub instead of fault.py
…pen reads the workspace client from context Co-authored-by: Isaac <no-reply@databricks.com>
…hem from DeploymentBundle The phases now reach DMS through StateDB (DmsClient, OperationBuffer, TakeOperationBuffer) rather than DeploymentBundle mirror fields. The operation drain stays before CompleteVersion. Co-authored-by: Isaac <no-reply@databricks.com>
…ffer in StateDB - add dstate.StorageBackend (WORKSPACE_FILESYSTEM/DEPLOYMENT_METADATA_SERVICE), set by Open from the state marker; the plan carries its string form so deploy --plan can reject a plan whose target switched backends - StateDB owns the recording lifecycle: RecordingError() and CompleteVersion() (idempotent via versionCompleted), with the operation-buffer drain moved into Finalize; drop the exposed OperationBuffer/TakeOperationBuffer getters and the recorder() indirection - gate DMS behavior on StorageBackend()==DEPLOYMENT_METADATA_SERVICE / ConfiguresDeploymentHistory instead of DmsClient()!=nil - consolidate plan validation in validatePlan (config-vs-plan backend match + stale-plan guard) Co-authored-by: Isaac <no-reply@databricks.com>
…ate unbind, restore v3 read - rename StampDeploymentID -> StampDeploymentIdForFirstVersion (only used for the first version's id) - inline RecordDeploymentHistoryEnv into RecordsDeploymentHistory (single predicate) - unbind: drop the config-based phase check; DeploymentBundle.Unbind opens without recording, so the state marker (not the config flag) refuses a recorded deployment - restore forward-compat read of featureStateVersion=3: this CLI still writes v2, but reads a v3 state with no features and refuses one that carries a feature Co-authored-by: Isaac <no-reply@databricks.com>
Extend declined-deploy, depends-on and deployment-metadata-change to capture the serialized 'bundle plan -o json' (now carrying storage_backend), the DMS list-resources view, and the raw recorded state. deployment-metadata-change pins state_path, which print_state.py cannot resolve, so it relies on list-resources. Co-authored-by: Isaac <no-reply@databricks.com>
…ate across the suite Extend no-resources, multiple-resources, record, successful-recreate, no-drift, stale-plan, emptied-resource and provenance to capture the serialized 'bundle plan -o json' (now carrying storage_backend), the DMS list-resources view, and the raw recorded state at each steady-state point. no-drift shows only the updated resource bumping to version 2; successful-recreate shows the recreate at version 2; provenance masks the git commit. Co-authored-by: Isaac <no-reply@databricks.com>
…-version golden
- deploy.go: StorageBackend() asserts the state is open, but only the direct engine
opens it, so gate the DMS deployment-create block on stateEngine.IsDirect() first.
Fixes a panic ("DeploymentState must be opened first") on terraform deploys, which
the direct-only dms tests did not exercise.
- future_version: supported state version is now 3 (v3 is read-forward-compatible),
so the too-new message reports 3.
Co-authored-by: Isaac <no-reply@databricks.com>
…version on recording error - C11: StateDB holds DeploymentID/LatestVersionID (set at Open from the service); CalculatePlan stamps the plan's lineage from StateDB instead of the bundle config tree (config.History stays only for 'bundle summary'). Behavior-neutral. - unbind: Open returns a sentinel ErrUnsettingRecording; DeploymentBundle.Unbind maps it to an unbind-specific message instead of the deploy/destroy-flavored guard text. - CompleteVersion now completes as Failure when the drained buffer holds a recording error, even if the caller passed success=true, so a destroy whose uploads failed keeps its deployment record. Co-authored-by: Isaac <no-reply@databricks.com>
… the readplan option The dedicated readplan test duplicated stale-plan, which already deploys from a saved raw plan under recording. Delete it and add the standard READPLAN matrix to stale-plan so its initial deploy runs both from the saved plan and recomputed - the deploy is untraced so the variant-dependent command stays out of the golden. Recorded operations still carry the deployment_id/version_id lineage (asserted in depends-on and others). Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
Extend the failure-injection tests to show how DMS records a failed or refused deploy: failed-delete/recreate/update, failed-update-permissions, operation-upload-fails-once, record-failure, version-never-created (and a WORKSPACE_FILESYSTEM plan on existing-state's first deploy). Each captures the healthy recorded state and the recorded result of the failure. Co-authored-by: Isaac <no-reply@databricks.com>
… wording - rename StartRecording -> InitializeOperationBuffer (it opens the buffer; kept separate from Open because a first deploy's id does not exist until after approval) - collapse the duplicated deployment/version fields to one public pair (DeploymentID/LatestVersionID); InitializeOperationBuffer records the first-deploy id, CompleteVersion derives the version from it - RecordingError is now a no-op unless the backend is the deployment metadata service, keeping the non-recording path cleanly separate (like RecordFailure) - stop surfacing the internal "deployment metadata service" name in user-facing errors; say "deployment history feature enabled" / "the deployment history service" - drop the "in place of nil client checks" comment and restore the original featureStateVersion wording (minimal C12 diff) Co-authored-by: Isaac <no-reply@databricks.com>
…ploy test Three bugs introduced by the previous hand-edit commit: 1. unlockedOpen: the 3-way switch reconciling config vs state was replaced with a simple `if !stateHasRecordDeploymentHistory && withDeploymentHistory`, which errored even on brand-new deployments (no state file → no feature flag → error). Restored the original logic including the bootstrap case for `recording && !recorded && len(db.Data.State) == 0`. 2. unlockedOpen: recording opens must delete a leftover WAL (the service owns the resources; a local log from a crash is irrelevant). The hand edit changed this to "ignore the WAL", breaking version-never-created and existing-state. Restored os.Remove on recording opens. 3. Error message indentation: the multi-line error had tab-indented continuation lines instead of no leading whitespace, mismatching the existing-state golden. Also: declined-deploy/script - add --get to the second print_requests call so the GET requests made during the declined deploy (GetDeployment + ListResources) appear in the output. Move out.requests.txt cleanup into the EXIT trap so the catalog delete's request file is removed and no out.requests.txt is left behind. Co-authored-by: Isaac <no-reply@databricks.com>
Remove | nostamp.py from all bundle/dms test scripts. nostamp was stripping deployment_id, next_version_id, and last_version_id from plan output (its purpose is to make non-DMS tests agnostic to recording mode). DMS-specific tests should assert these fields directly — they are the feature under test. Regenerate all goldens. Plans now show deployment_id/next_version_id/ last_version_id after the first deploy, confirming the fields are serialized. Co-authored-by: Isaac <no-reply@databricks.com>
Add --no-dms flag to print_state.py: prints resources.json as-is without
fetching state from the deployment metadata service. The existing behaviour
(enriching the output with DMS-held resources) exists for non-DMS tests so
their goldens stay identical whether recording is on or off; DMS tests need
the opposite — to assert that the local file IS the tombstone (empty state,
just the header + record_deployment_history feature flag).
Update all bundle/dms test scripts to use print_state.py --no-dms.
Regenerate goldens: state blocks that previously showed full resource state
now show "state": {}, confirming the service holds the resources and the
local file carries only the tombstone marker.
Co-authored-by: Isaac <no-reply@databricks.com>
…re-deploy plan in depends-on
Add --no-dms flag to print_state.py: prints resources.json as-is without
fetching state from the deployment metadata service. The existing behaviour
(enriching the output with DMS-held resources) exists for non-DMS tests so
their goldens stay identical whether recording is on or off; DMS tests need
the opposite — to assert that the local file IS the tombstone (empty state,
just the header + record_deployment_history feature flag).
Update all bundle/dms test scripts to use print_state.py --no-dms.
Regenerate goldens: state blocks now show "state": {}, confirming the
service holds the resources and the local file carries only the tombstone.
Also add a bundle plan -o json call before the first deploy in depends-on,
showing that no deployment_id or version fields appear before any deploy exists.
Co-authored-by: Isaac <no-reply@databricks.com>
…te WAL header The leftover WAL now carries the feature flag, so the test proves the WAL is discarded rather than replayed: if it were merged the feature would leak into the refused deploys, but the refusals still stand and the final fresh deploy's feature comes from config, with resources.json.wal gone (--expect 0). Co-authored-by: Isaac <no-reply@databricks.com>
…change These two section-header titles printed to stdout immediately before a command whose trace line goes to stderr. On cloud the two streams interleave nondeterministically and the title line is dropped, so the golden never matched. The surrounding >>> commands and their output already convey the state, so the titles are removed rather than fixing the shared title() helper (which would add a blank line after every title in every acceptance golden). Co-authored-by: Isaac <no-reply@databricks.com>
In DMS/direct mode only resources.json exists among the state files, so the filename check added nothing; the records_deployment_history() + --no-dms gate alone selects the recorded-state path. Co-authored-by: Isaac <no-reply@databricks.com>
…tions
Add a ListOperations handler to the testserver (GET
/api/2.0/bundle/deployments/{id}/versions/{version}/operations) mirroring the
service, and call bundle-deployments list-operations in failed-delete to assert
the failed delete operation carries its error message and FAILED status.
Co-authored-by: Isaac <no-reply@databricks.com>
…omment Reverts a comment rewording; the longer original phrasing is clearer about why a featureStateVersion state with no features is accepted without running the migrations and its on-disk version left as-is. Co-authored-by: Isaac <no-reply@databricks.com>
…og (cloud-enabled) Drop the fault.py injection; point the recreated schema at a catalog that does not exist so the create half of the recreate fails on its own. The testserver now models catalog existence: SchemasCreate returns 404 CATALOG_DOES_NOT_EXIST when the parent catalog is absent, and the fake seeds the "main" catalog so the common case (schemas in main) keeps working. This makes the failure identical locally and on a real workspace, so the test runs with Cloud = true. Co-authored-by: Isaac <no-reply@databricks.com>
…ppingVersion Re-add the test pinning the featureStateVersion special case: a v3 state with no features is accepted and left at v3 (not flipped to currentStateVersion), while a v3 state recording any feature is refused with the upgrade hint. It is the forcing function for the deferred version bump. Co-authored-by: Isaac <no-reply@databricks.com>
RecordingError only reads operationBuffer and calls buf.Err() (a quick read on the buffer's own mutex), so holding db.mu across it via defer is safe and reads cleaner. CompleteVersion keeps its manual unlock: the network work after the snapshot must run outside the lock. Co-authored-by: Isaac <no-reply@databricks.com>
…history
New test showing bind and unbind against a bundle that records deployment
history. Before any deploy there is no recorded state, so bind just fails to
find the workspace resource and unbind is a no-op. Once a deployment is
recorded, both are refused ("bind/unbind is not supported for a bundle that
records deployment history"): the service owns the resource set, so adopting or
releasing a resource by editing local state is not allowed. Local-only, since
the refusal is a client-side gate.
Co-authored-by: Isaac <no-reply@databricks.com>
RecordOperation and RecordFailure each took db.mu separately to touch latestState and then again inside record to touch pending, so the two maps could be observed mid-update. Move the latestState read/write into record, guarded by the same lock as the pending update: a state write (isStateWrite) records the latest state, a failure merges its status/message onto that recorded state (or falls back to the passed update). RecordOperation and RecordFailure no longer lock directly. Co-authored-by: Isaac <no-reply@databricks.com>
…ull -o json Add a dms_plan helper (bundle/dms/script.prepare) that prints the plan JSON with the per-resource plan body dropped (jq del(.plan)) — leaving the header fields cli_version, lineage, serial, deployment_id, next/last_version_id, storage_backend — then runs bundle plan again for the readable summary. Replace every full `bundle plan -o json` dump in the bundle/dms tests with it. The per-resource plan carried remote_state that diverges between the fake and a real workspace (e.g. a serverless pipeline's effective_serverless_compute_id), which broke no-drift on cloud; dropping it keeps local and cloud identical while still asserting the DMS header fields and the human-readable plan. Co-authored-by: Isaac <no-reply@databricks.com>
…xistingUpdateIfAny Trim the verbose doc comments to a couple of plain lines, and rename record's flag from isStateWrite to keepExistingUpdateIfAny (inverting it): true merges a failure onto the resource's last recorded state if one exists, false saves the update as that latest state. Co-authored-by: Isaac <no-reply@databricks.com>
Create a real workspace job and bind to it, so bind gets past fetching the resource and reaches the config gate that refuses bind while recording deployment history - recording that error instead of "Job 123 does not exist". Co-authored-by: Isaac <no-reply@databricks.com>
…nown bug, Cloud=false Emptying a grants node makes apply call DeleteState, recording a succeeded UPDATE that names no state while the schema still exists; a real workspace rejects that (400 INVALID_PARAMETER_VALUE). The fake does not model the check, so the recorded output is the buggy behavior. Disable the cloud run and document the fix needed (emptying grants should record an update carrying the emptied state, not a delete). Co-authored-by: Isaac <no-reply@databricks.com>
…a list-operations Add a bundle-deployments list-operations call after the failed permissions update so the golden asserts the failed operation (jobs.foo.permissions, UPDATE, FAILED) carries the 403 error message the service recorded. Co-authored-by: Isaac <no-reply@databricks.com>
Dropped as unnecessary; operation-upload-fails-once still covers the upload-failure path. Co-authored-by: Isaac <no-reply@databricks.com>
Both duplicate coverage in the remaining bundle/dms tests: recording calls and serialized operation state are exercised by successful-recreate, multiple-resources, depends-on and others; a failed update is covered by failed-update-permissions and failed-recreate. Reword the two comments that pointed at bundle/dms/record. Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
When opening a deployment state with experimental.record_deployment_history enabled, the service is the source of truth for resources, not the local state file. Nullify the local state so it acts as a tombstone (carrying only the feature marker and header). This allows stale local state from a destroyed DMS deployment to bootstrap a fresh deployment, instead of erroring with "this deployment already exists and is not recorded". The fix removes the guard that previously prevented enabling DMS on deployments with local resources. This is a necessary tradeoff to support the common scenario where a DMS deployment is destroyed on the service but the local state cache remains. The service is now authoritative: - If dmsDeploymentID is non-empty: ListResources fetches the service's resources - If dmsDeploymentID is empty: no resources are loaded (fresh/destroyed deployment) Tradeoff: this also allows enabling DMS on an existing non-DMS deployment, which could leave old resources orphaned if they have different IDs or names. Users should destroy a non-DMS deployment before enabling DMS to avoid this scenario. Accept this tradeoff to unblock the more common case (destroyed DMS bootstrap). Co-authored-by: Isaac <no-reply@databricks.com>
Collaborator
Integration test reportCommit: 0336c85
29 interesting tests: 27 FAIL, 1 KNOWN, 1 SKIP
|
shreyas-goenka
force-pushed
the
isaac/pr6052-fixes
branch
2 times, most recently
from
September 8, 2026 13:57
831045c to
e9dbe85
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a bug where enabling DMS recording (
experimental.record_deployment_history) on a deployment fails when the local state cache still has resources, even if the remote deployment was destroyed. The fix nullifies local state when recording is enabled, treating the state file as a tombstone that only carries the feature marker and header.The Problem
When a DMS deployment is destroyed on the service, the local
resources.jsonis a cache that can retain stale resources. Attempting to redeploy with DMS enabled would fail with:This error was caused by a guard that checked
recording && !recorded && len(db.Data.State) > 0. While the guard was originally designed to prevent enabling DMS on existing non-DMS deployments (creating resources a second time), it also blocked the legitimate case of a destroyed DMS deployment being rebuilt.The Solution
When opening a deployment state with recording enabled, nullify the local state. The service becomes the source of truth:
dmsDeploymentIDis non-empty:ListResourcesfetches resources from the servicedmsDeploymentIDis empty: no resources are loaded (fresh or destroyed deployment)This allows the bootstrap path to be taken (
recording && !recorded && len(db.Data.State) == 0) instead of erroring.Tradeoff
Nullifying local state removes the guard against enabling DMS on an existing non-DMS deployment (no service node, but resources cached locally). This could:
However, this is an acceptable tradeoff to unblock the more common and important case: recovering destroyed DMS deployments. Users who need to migrate from non-DMS to DMS should explicitly destroy the bundle first.
Changes
bundle/direct/dstate/state.go: Nullify local state whenrecordingis truebundle/direct/dstate/state_test.go: Add unit test verifying preconditionsacceptance/bundle/dms/existing-state/output.txt: Updated golden (reflecting changed guard behavior)Testing
go test ./bundle/direct/dstate/...This pull request and its description were written by Isaac.